spec/controllers/admin/groups_controller_spec.rb (2)

52-58: Hardcoded ID=1 assumes a specific automatic group exists.

This test relies on Group.find(1) returning an automatic group. Consider using Fabricate(:group, automatic: true) for test isolation, similar to the .destroy context.

♻️ Suggested improvement

     it "ignore name change on automatic group" do
-      xhr :put, :update, id: 1, name: "WAT", visible: "true"
+      group = Fabricate(:group, automatic: true)
+      original_name = group.name
+      xhr :put, :update, id: group.id, name: "WAT", visible: "true"
       response.should be_success

-      group = Group.find(1)
+      group.reload
-      group.name.should_not == "WAT"
+      group.name.should == original_name
       group.visible.should == true
     end

---

93-96: Same hardcoded ID concern for automatic group test.

Consider fabricating an automatic group instead of relying on ID=1.

app/controllers/admin/groups_controller.rb (2)

71-75: Silent failure when username is not found.

Invalid usernames are silently ignored. Consider collecting failures and returning feedback to the admin so they know which usernames were not added.

♻️ Suggested improvement with error feedback

+    not_found = []
     usernames.split(",").each do |username|
       if user = User.find_by_username(username)
         group.add(user)
+      else
+        not_found << username
       end
     end

     if group.save
-      render json: success_json
+      render json: success_json.merge(not_found: not_found)
     else

---

72-72: Wrap assignment in parentheses for clarity.

RuboCop flags the assignment-in-condition. While this is a valid Ruby pattern, wrapping in parentheses makes the intent explicit.

♻️ Proposed fix

-      if user = User.find_by_username(username)
+      if (user = User.find_by_username(username))

app/assets/javascripts/discourse/models/group.js (2)

59-60: Missing semicolon after addMembers function.

The closing of the then() callback is missing a semicolon for consistency with the rest of the codebase.

♻️ Proposed fix

     }).then(function() {
       // reload member list
       self.findMembers();
-    })
+    });
   },

---

40-60: Consider propagating errors from member operations.

Both removeMember and addMembers call findMembers() in the success callback but don't handle or propagate errors from the AJAX call. The caller has no way to know if the operation failed.

♻️ Example with error propagation

removeMember: function(member) {
  var self = this;
  return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
    type: "DELETE",
    data: { user_id: member.get("id") }
  }).then(function() {
    self.findMembers();
  }).catch(function(error) {
    // Allow caller to handle errors
    throw error;
  });
},

app/assets/javascripts/admin/controllers/admin-group.js.es6 (1)

61-65: Consider clearing the usernames input after a successful add.
This avoids accidental duplicate submissions and matches the TODO.

🧹 Possible follow-up

-      this.get("model").addMembers(this.get("usernames"));
+      var self = this;
+      this.get("model").addMembers(this.get("usernames")).then(function() {
+        self.set("usernames", null);
+      });

If `addMembers` is not promise-based, I can adapt this to the correct callback pattern—just point me to the model API.